Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add System.PosixCompat.Process #6

Closed
wants to merge 1 commit into from
Closed

Conversation

ners
Copy link
Contributor

@ners ners commented Nov 2, 2023

This PR adds the getProcessID function which is supported on both Unix and Win32 systems. It is already used by lsp-test and lsp-client.

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution.

Please add a testcase demonstrating the use of this new module.

src/System/PosixCompat/Process.hs Outdated Show resolved Hide resolved
src/System/PosixCompat/Process.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ners
Copy link
Contributor Author

ners commented Nov 4, 2023

Thank you for the speedy review, @andreasabel. I've added a very simple test, though I'm not entirely sure what I could assert in it.
Could you please run the CI again?

@ners ners requested a review from andreasabel November 4, 2023 00:12
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test!

Comment: You seem to never run this on Windows locally, so there is still a build failure due to a typo...

Anyway, I think this can be merged once CI passes.

src/System/PosixCompat/Process.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@andreasabel andreasabel added this to the 0,7,1 milestone Nov 5, 2023
@andreasabel andreasabel added the enhancement New feature or request label Nov 5, 2023
@ners ners force-pushed the main branch 2 times, most recently from c41084d to 42c0d02 Compare November 6, 2023 08:08
@ners
Copy link
Contributor Author

ners commented Nov 6, 2023

@andreasabel apologies for the back-and-forth, I was hoping the CI would run automatically on PR. I've now discovered that I can enable CI actions on my own fork, so in the future I can test locally before opening a PR. 🙂

@ners
Copy link
Contributor Author

ners commented Nov 8, 2023

@andreasabel anything else to be done here or can we merge?


{-|
This module makes the operations exported by @System.Posix.Process@
available on all platforms. On POSIX systems it re-exports operations from
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is a more a statement of intent than of actual function.
Currently, it only re-exports a single operation from System.Posix.Process.
Maybe it could be reformulated as This module intends to make the operations of @System.Posix.Process@ available on all platforms. What do you think?

Copy link
Contributor Author

@ners ners Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good; I’m in favour of iteratively improving the docs. Before the next version we could consider rewording the other modules as well.

@andreasabel
Copy link
Member

Do you want this to be released asap or only eventually?

@ners
Copy link
Contributor Author

ners commented Nov 9, 2023

Ideally I’d like version 0.7.1 sooner rather than later so I can update lsp-client.

What I’d like to discuss afterwards is potentially refactoring the partially supported functions (e.g. the ones that throw exceptions on Windows) to a separate module in preparation for 0.8.

@andreasabel
Copy link
Member

I removed the Haskell warning for shadowing id.
I could not push to your branch (maybe because you used main rather than a dedicated one on your fork), so I moved this PR to

@andreasabel andreasabel closed this Dec 4, 2023
@andreasabel andreasabel self-assigned this Dec 4, 2023
@andreasabel
Copy link
Member

Released in 0.7.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants